Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

perf(ngcc): read dependencies from entry-point manifest #36486

Conversation

petebacondarwin
Copy link
Member

@petebacondarwin petebacondarwin commented Apr 7, 2020

Previously, even if an entry-point did not need to be processed,
ngcc would always parse the files of the entry-point to compute
its dependencies. This can take a lot of time for large node_modules.

Now these dependencies are cached in the entry-point manifest,
and read from there rather than computing them every time.

See #36414 (comment)
FW-2047

@petebacondarwin petebacondarwin added area: performance action: review The PR is still awaiting reviews from at least one requested reviewer refactoring Issue that involves refactoring or code-cleanup comp: ngcc labels Apr 7, 2020
@ngbot ngbot bot modified the milestone: needsTriage Apr 7, 2020
@pullapprove pullapprove bot requested a review from kara April 7, 2020 18:57
@petebacondarwin petebacondarwin force-pushed the ngcc-no-dep-check-for-processed-entry-points branch from b8f1f8a to 2734b3e Compare April 7, 2020 19:57
Copy link
Member

@JoostK JoostK left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

for (const basePath of this.basePaths) {
const entryPoints = this.entryPointManifest.readEntryPointsUsingManifest(basePath) ||
this.walkBasePathForPackages(basePath);
unsortedEntryPoints.push(...entryPoints);
entryPoints.forEach(e => unsortedEntryPoints.push(e));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this change really needed? I prefer the original code.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I was looking at the generated JS code for this library, I noticed that the current code gets downleveled to (line 43):

unsortedEntryPoints.push.apply(unsortedEntryPoints, __spread(entryPoints));

See this playground

where __spread() is implemented like:

var __read = (this && this.__read) || function (o, n) {
    var m = typeof Symbol === "function" && o[Symbol.iterator];
    if (!m) return o;
    var i = m.call(o), r, ar = [], e;
    try {
        while ((n === void 0 || n-- > 0) && !(r = i.next()).done) ar.push(r.value);
    }
    catch (error) { e = { error: error }; }
    finally {
        try {
            if (r && !r.done && (m = i["return"])) m.call(i);
        }
        finally { if (e) throw e.error; }
    }
    return ar;
};
var __spread = (this && this.__spread) || function () {
    for (var ar = [], i = 0; i < arguments.length; i++) ar = ar.concat(__read(arguments[i]));
    return ar;
};

This seemed to me highly sub-optimal and the approach in this PR is much simpler - most likely faster and certainly creates less memory pressure.


This brings up a side-point. If ngcc (and ngc) is always going to be run with node.js >= 10.0 then perhaps we should change our downleveling settings?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that's a fair point. __spread usually shows up in profiling sessions quite noticeable so I would love to get rid of it. Emitting to ES2015 seems like the best way to achieve that, indeed.

@petebacondarwin petebacondarwin force-pushed the ngcc-no-dep-check-for-processed-entry-points branch from 2734b3e to 7fd01da Compare April 8, 2020 10:16
Copy link
Member

@gkalpak gkalpak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be great to see some numbers. Other than that looks great ✨

Previously, even if an entry-point did not need to be processed,
ngcc would always parse the files of the entry-point to compute
its dependencies. This can take a lot of time for large node_modules.

Now these dependencies are cached in the entry-point manifest,
and read from there rather than computing them every time.

See angular#36414
FW-2047
@petebacondarwin petebacondarwin force-pushed the ngcc-no-dep-check-for-processed-entry-points branch from 7fd01da to ddb50af Compare April 9, 2020 09:11
@petebacondarwin petebacondarwin removed the request for review from kara April 9, 2020 10:01
@petebacondarwin petebacondarwin added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Apr 9, 2020
@petebacondarwin
Copy link
Member Author

petebacondarwin commented Apr 9, 2020

Initial tests on AIO... I first built all the packages in the node_modules directory by running ngcc -l debug. Then ran the same command again a few times, which represents the "noop" ngcc run.

  Master PR
Full 308.61 321.08
No manifest 4.27 6.68
noop 1 5.26 1.21
noop 2 5.16 1.72
noop 3 4.92 1.78
noop 4 4.46 1.75
Average noop 4.95 1.615

So it can be seen that there is no real difference if the manifest has to be created.
But where there is a manifest, you can see that this PR is about 3x faster than master.

@petebacondarwin petebacondarwin added the target: patch This PR is targeted for the next patch release label Apr 9, 2020
The base path for package and entry-points is known so there is
no need to store these in the file. Also this commit avoids storing
empty arrays unnecessarily.
@petebacondarwin petebacondarwin force-pushed the ngcc-no-dep-check-for-processed-entry-points branch from a320438 to 93d7b5d Compare April 9, 2020 15:03
@petebacondarwin
Copy link
Member Author

Further improvements, particularly only loading the Transformer code if it is actually required knocks a bit more time off the noop runs:

  Master PR (no lazy) PR (with lazy)
noop 1 5.26 1.21 0.97
noop 2 5.16 1.72 0.96
noop 3 4.92 1.78 0.89
noop 4 4.46 1.75 0.97
Average noop 4.95 1.615 0.9475

@petebacondarwin petebacondarwin removed the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Apr 9, 2020
@petebacondarwin
Copy link
Member Author

@JoostK and @gkalpak - I added a couple of commits. PTAL

Comment on lines +186 to +188
Array<AbsoluteFsPath>?,
Array<AbsoluteFsPath|PathSegment>?,
Array<AbsoluteFsPath>?,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to use relative paths for these too?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about it for dependencies and deep-imports even though there is a chance that they might start with .. (but that probably doesn't matter). But I didn't want to do it for missing since some of these are PathSegments but relative to the import place in the code, so we couldn't be sure, when reading in the manifest, whether we are reading in an AbsolutePath that needs de-relativising or a PathSegment that needs to be left alone.

@@ -315,6 +314,7 @@ export function mainNgcc({
const createCompileFn: CreateCompileFn = onTaskCompleted => {
const fileWriter = getFileWriter(
fileSystem, logger, pkgJsonUpdater, createNewEntryPointFormats, errorOnFailedEntryPoint);
const {Transformer} = require('./packages/transformer');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume we can do more of this. Is Trasnformer special in some way (e.g. particularly heavy to load)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, Transformer brings in all the trait compiler stuff, all the reflection hosts etc.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked at other imports but most of them are needed in the top level main function :-/

@petebacondarwin petebacondarwin added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Apr 9, 2020
atscott pushed a commit that referenced this pull request Apr 9, 2020
Previously, even if an entry-point did not need to be processed,
ngcc would always parse the files of the entry-point to compute
its dependencies. This can take a lot of time for large node_modules.

Now these dependencies are cached in the entry-point manifest,
and read from there rather than computing them every time.

See #36414
FW-2047

PR Close #36486
atscott pushed a commit that referenced this pull request Apr 9, 2020
The base path for package and entry-points is known so there is
no need to store these in the file. Also this commit avoids storing
empty arrays unnecessarily.

PR Close #36486
atscott pushed a commit that referenced this pull request Apr 9, 2020
@atscott atscott closed this in a185efb Apr 9, 2020
atscott pushed a commit that referenced this pull request Apr 9, 2020
The base path for package and entry-points is known so there is
no need to store these in the file. Also this commit avoids storing
empty arrays unnecessarily.

PR Close #36486
atscott pushed a commit that referenced this pull request Apr 9, 2020
@petebacondarwin petebacondarwin deleted the ngcc-no-dep-check-for-processed-entry-points branch April 9, 2020 19:35
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators May 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker area: performance cla: yes refactoring Issue that involves refactoring or code-cleanup target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants